Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Load chats when handling browser notifications. Fix initial sign in bug. #339

Merged
merged 6 commits into from
Aug 28, 2020
Merged

Load chats when handling browser notifications. Fix initial sign in bug. #339

merged 6 commits into from
Aug 28, 2020

Conversation

marcaaron
Copy link
Contributor

Fixes:
#334
#316

Tests:

Testing first sign in

  1. Completely log out of ChatDot and clear localStorage
  2. Login and verify it is successful and that you don't need to refresh to get everything working and there are no errors in JS console

Testing a new chat report notification will add the chat to your LHN if it doesn't exist
Apply this diff or run the app in production mode

diff --git a/src/lib/actions/Report.js b/src/lib/actions/Report.js
index 90ffc65..6973b45 100644
--- a/src/lib/actions/Report.js
+++ b/src/lib/actions/Report.js
@@ -150,9 +150,9 @@ function updateReportWithNewAction(reportID, reportAction) {
             // This is necessary for local development because there will be pusher events from other engineers with
             // different reportIDs. This means that while in development it's not possible to make new chats appear
             // by leaving creating chats and leaving comments in other windows
-            if (!CONFIG.IS_IN_PRODUCTION && !ionReportID) {
-                throw new Error('report does not exist in the store, so ignoring new comments');
-            }
+            // if (!CONFIG.IS_IN_PRODUCTION && !ionReportID) {
+            //     throw new Error('report does not exist in the store, so ignoring new comments');
+            // }
  1. Login with Account A in one browser session
  2. Login with Account B in another browser session
  3. Create a chat report between Account A and B via www JS console API.createChatReport({emailList:'address1,address2'})
  4. Refresh Account B but do not refresh Account A
  5. Locate the chat in Account B's ChatDot window and add a comment to it
  6. Verify that Account A gets a notification and clicking on it brings them to the chat + it now appears in the LHN

@marcaaron marcaaron self-assigned this Aug 28, 2020
@marcaaron marcaaron changed the title Marcaaron show new conversations in lhn Load chats when handling browser notifications. Fix initial sign in bug. Aug 28, 2020
tgolen
tgolen requested changes Aug 28, 2020
View reviewed changes
src/lib/actions/Report.js
*/
function fetchChatReports() {
return queueRequest('Get', {returnValueList: 'chatList'})
.then(({chatList}) => fetchChatReportsByIDs(String(chatList).split(',')));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it necessary to cast this to a string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because rvl=chatList returns an int when there's only one chat and calling .split() on a Number doesn't work. Can probably handle this a different way, but imagining that we'll change the way we fetch chats in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that's good to know! That would be another good thing to add as a comment in the code. It could bite someone.

src/lib/actions/Report.js
throw new Error('report does not exist in the store, so ignoring new comments');
}

if (!ionReportID) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a comment here to explain how it gets into this situation?

@marcaaron
Copy link
Contributor Author

Updated

@tgolen tgolen merged commit 2f6ed62 into master Aug 28, 2020
@tgolen tgolen deleted the marcaaron-showNewConversationsInLHN branch August 28, 2020 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgolen tgolen tgolen approved these changes

@cead22 cead22 Awaiting requested review from cead22

Assignees

@marcaaron marcaaron

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@marcaaron @tgolen